Skip to content

Move snapshot data write to async mode. #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 21, 2025
Merged

Conversation

xiaoxichen
Copy link
Collaborator

In order to provider higher queue depth for disk, which is optimal for scheduler to merge requests.

Also IO and compute(checksum) can parallel.

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 59.79381% with 39 lines in your changes missing coverage. Please review.

Project coverage is 61.14%. Comparing base (1746bcc) to head (7ba0688).
Report is 74 commits behind head on main.

Files with missing lines Patch % Lines
...lib/homestore_backend/snapshot_receive_handler.cpp 48.21% 24 Missing and 5 partials ⚠️
src/lib/homestore_backend/pg_blob_iterator.cpp 74.35% 9 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #299      +/-   ##
==========================================
- Coverage   63.15%   61.14%   -2.02%     
==========================================
  Files          32       34       +2     
  Lines        1900     3418    +1518     
  Branches      204      407     +203     
==========================================
+ Hits         1200     2090     +890     
- Misses        600     1109     +509     
- Partials      100      219     +119     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xiaoxichen xiaoxichen requested review from yuwmao and koujl May 15, 2025 06:18
@yuwmao
Copy link
Contributor

yuwmao commented May 15, 2025

If a blob batch returns an error due to ALLOC_BLK_ERR, the async_write might still be in progress.
Then when the retry batch with the same blobs comes, is there any problem? Seems no correctness issue due to the duplication handling but could have some space waste(the previous write might not be committed in indexservice when second write happens)?

@xiaoxichen xiaoxichen force-pushed the br_opt branch 2 times, most recently from f1f2fe8 to 0501da0 Compare May 19, 2025 08:41
@xiaoxichen xiaoxichen force-pushed the br_opt branch 4 times, most recently from 75a28df to 8b6d382 Compare May 20, 2025 08:32
In order to provider higher queue depth for disk, which is optimal
for scheduler to merge requests.

Also IO and compute(checksum) can parallel.

Signed-off-by: Xiaoxi Chen <[email protected]>
This make the function more closure and retryable.

Signed-off-by: Xiaoxi Chen <[email protected]>
Signed-off-by: Xiaoxi Chen <[email protected]>
deleted and dup blobs should be counted.

Signed-off-by: Xiaoxi Chen <[email protected]>
Signed-off-by: Xiaoxi Chen <[email protected]>
bool success =
home_obj_.local_add_blob_info(ctx_->pg_id, BlobInfo{ctx_->shard_cursor, blob_id, blk_id});
if (!success) {
LOGE("Failed to add blob info for blob_id={}", blob_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to free blk if commit_blk or add index fail?

if (status != homestore::BlkAllocStatus::SUCCESS) {
LOGE("Failed to allocate blocks for shardID=0x{:x}, pg={}, shard=0x{:x} blob {}", ctx_->shard_cursor,
(ctx_->shard_cursor >> homeobject::shard_width), (ctx_->shard_cursor & homeobject::shard_mask),
blob->blob_id());
std::unique_lock< std::shared_mutex > lock(ctx_->progress_lock);
ctx_->progress.error_count++;
return ALLOC_BLK_ERR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to add a flip to simulate alloc error.

Signed-off-by: Xiaoxi Chen <[email protected]>
Copy link
Contributor

@yuwmao yuwmao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xiaoxichen xiaoxichen merged commit 0a7661d into eBay:main May 21, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants